feat: Parallel include pointers #9861
Conversation
…moumouls/concurrent-include # Conflicts: # package.json
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughThe changes implement parallelization of included object loading in Parse queries by reworking the Changes
Sequence DiagramssequenceDiagram
participant Q as Query Handler
participant H as handleInclude (async)
participant E as Execution Tree
participant F1 as Fetch Path 1
participant F2 as Fetch Path 2
participant R as Results
Q->>H: Trigger include processing
H->>E: Build traversal tree from include paths
par Parallel Fetches
E->>F1: Execute independent path 1
E->>F2: Execute independent path 2
and
F1->>E: Resolve with sub-results
F2->>E: Resolve with sub-results
end
E->>H: All paths resolved
H->>R: Hydrate and merge included objects
H->>Q: Return completed results
rect rgba(200, 220, 255, 0.3)
note over E,R: Nested includes resolved after parent<br/>fetch completes, enabling dependent paths
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (8)📓 Common learnings📚 Learning: 2025-10-16T19:27:05.311ZApplied to files:
📚 Learning: 2025-08-26T14:06:31.853ZApplied to files:
📚 Learning: 2025-08-27T12:33:06.237ZApplied to files:
📚 Learning: 2025-04-30T19:31:35.344ZApplied to files:
📚 Learning: 2025-05-09T09:59:06.289ZApplied to files:
📚 Learning: 2025-05-09T09:59:06.289ZApplied to files:
📚 Learning: 2025-05-04T20:41:05.147ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #9861 +/- ##
==========================================
+ Coverage 93.07% 93.08% +0.01%
==========================================
Files 187 187
Lines 15224 15234 +10
Branches 177 177
==========================================
+ Hits 14170 14181 +11
+ Misses 1042 1041 -1
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…moumouls/concurrent-include # Conflicts: # .gitignore
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/RestQuery.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.853Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.
📚 Learning: 2025-08-26T14:06:31.853Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.853Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.
Applied to files:
src/RestQuery.js
Moumouls
changed the title
|
So @mtrezza depending of DB co location with the server or even DB performance ( under load, slow queries), this PR introduce a important optimization in case of 10ms DB latency it's already a 23x time faster 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
spec/RestQuery.spec.js (2)
390-390: Consider a more descriptive test name.The term "battle test" is informal. A clearer name would better describe the test's purpose and expected behavior.
Consider renaming to something like:
- it('battle test parallel include with 100 nested includes', async () => { + it('should efficiently load 100 nested includes in parallel', async () => {
445-447: Remove debug timing statements.The
console.timeandconsole.timeEndcalls are debug statements that clutter test output. If performance validation is needed, capture the timing in a variable and assert against a threshold; otherwise, remove these statements.Apply this diff to remove the timing statements:
- console.time('query.find'); const results = await query.find(); - console.timeEnd('query.find');Alternatively, if you want to validate performance:
+ const startTime = Date.now(); const results = await query.find(); + const duration = Date.now() - startTime; + expect(duration).toBeLessThan(5000); // Adjust threshold as needed - console.time('query.find'); - console.timeEnd('query.find');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/RestQuery.spec.js(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
spec/RestQuery.spec.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
spec/RestQuery.spec.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
spec/RestQuery.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
spec/RestQuery.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
spec/RestQuery.spec.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/RestQuery.spec.js
📚 Learning: 2025-08-26T14:06:31.853Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.853Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.
Applied to files:
spec/RestQuery.spec.js
🧬 Code graph analysis (1)
spec/RestQuery.spec.js (1)
spec/helper.js (2)
Parse(4-4)i(346-346)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.3
🔇 Additional comments (1)
spec/RestQuery.spec.js (1)
390-470: Excellent test coverage for parallel include functionality.This test comprehensively validates the parallel include feature by:
- Creating a realistic 3-level object hierarchy (1 root → 10 Level1 → 100 Level2)
- Testing 110 nested include paths simultaneously
- Verifying all nested objects are properly hydrated
- Using async/await patterns consistently (per Parse Server conventions)
The test directly addresses the PR objective to validate parallel include loading with deep nesting. The structure is clear and assertions are thorough.
Based on learnings
|
I've added a perf check in the CI. It runs only specific tests though, see docs. Do you want to add a benchmark to see the diff? Note that this is alpha, not sure whether it works reliably. I tested it with another PR and it showed no perf impact. However, it shows a perf regression issue for this PR here for creating objects. The other PR shows similar values for the baseline, so the baseline itself may be correct. It could be that the perf test algorithm has an issue (miscalculating an improvement for a regression). This PR only seems to change the |
It will not show much @mtrezza as you can see on my benchmark
|
|
Interested in this pr, Any eta on this? When can it come to v7/v8 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
benchmark/performance.js (1)
296-340: Suggest minor consistency and clarity improvements.The benchmark implementation is functionally correct. A few optional refinements:
Naming inconsistency: Line 297 says "Parallel Include Pointers" but line 335 uses "Query with Include (2 levels)". Consider using consistent terminology.
Redundant modulo operations: Lines 320 and 330 use
i % level2Objects.lengthandi % level1Objects.lengthwhereiranges from 0-9 and both arrays have length 10, making the modulo redundant.Query variation: Unlike
benchmarkSimpleQuery(which varies the query using a counter), all iterations here query the same data. Consider adding variation if you want to measure cache effects or query diversity.Example diff for consistency:
/** - * Benchmark: Query with Include (Parallel Include Pointers) + * Benchmark: Query with Include (2 levels) */Example diff for redundant modulo (lines 320, 330):
- obj.set('level2', level2Objects[i % level2Objects.length]); + obj.set('level2', level2Objects[i]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
benchmark/performance.js(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
benchmark/performance.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Code Analysis (javascript)
🔇 Additional comments (1)
benchmark/performance.js (1)
390-392: No duplicates found—benchmark code is correct.Verification confirms only one
benchmarkQueryWithIncludefunction definition and one invocation exist. The second regex match at line 299 is the function definition itself, not a duplicate. The invocation at line 392 follows the established benchmark pattern correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci-performance.yml(2 hunks)benchmark/db-proxy.js(1 hunks)benchmark/performance.js(6 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
benchmark/performance.js
🧬 Code graph analysis (2)
benchmark/db-proxy.js (1)
benchmark/performance.js (5)
require(14-14)require(15-15)require(34-34)server(148-156)server(444-444)
benchmark/performance.js (1)
benchmark/db-proxy.js (1)
server(15-54)
|
Hi @RahulLanjewar93, we just need to figure out the "project pipeline" support but i believe we will be able to merge this one quickly the target will be PS8 or PS9 |
|
Okay just adding my scenario here, We run parse server in a k8s environment. We have multiple pods running of parse server. To achieve load balancing, we set the direct access to false. Now suppose i do a query which returns 1k objects, i have included a pointer now the requests are sent individually both at db level and server level My idea is to somehow batch the requests made for includer pointer so there is less network round trip. And the dbs calls can be converted to a single db call as well using $in operator on the _is field in mongo Implementation might be difficult or even impossible, but generally this approach should be more performant |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
benchmark/performance.js (1)
37-37: Restore intended 10 ms latency constantThe
LATENCY_MSenvironment variable is set to'10000'(10 seconds), but line 43 and the surrounding context indicate the intent is 10 milliseconds. With 10-second latency and the default iteration count, the include benchmark would take many hours and cause CI timeouts.Apply this diff to fix the latency value:
- env: { ...process.env, PROXY_PORT: '27018', TARGET_PORT: '27017', LATENCY_MS: '10000' }, + env: { ...process.env, PROXY_PORT: '27018', TARGET_PORT: '27017', LATENCY_MS: '10' },
🧹 Nitpick comments (2)
benchmark/performance.js (2)
162-164: Wrapserver.close()in a promise for proper cleanup
server.close()initiates shutdown but completes asynchronously. Without awaiting its callback, the subsequent 500ms timeout may not guarantee complete cleanup, potentially causing port conflicts or resource leaks in subsequent benchmarks.Apply this diff to properly await server closure:
proxyServerCleanup = async () => { - server.close(); - await new Promise(resolve => setTimeout(resolve, 500)); + await new Promise(resolve => server.close(resolve)); proxyServerCleanup = null; };
503-503: Consider increasing cleanup timeout for proxy overheadWith
stopProxy()waiting 500ms and potential additional time for server cleanup, the 1000ms timeout may be close to the minimum required. Increasing to 1500-2000ms would provide a safer margin.Apply this diff if you want additional safety margin:
- setTimeout(() => process.exit(0), 1000); + setTimeout(() => process.exit(0), 1500);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci-performance.yml(2 hunks)benchmark/performance.js(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci-performance.yml
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
benchmark/performance.js
🧬 Code graph analysis (1)
benchmark/performance.js (1)
benchmark/db-proxy.js (1)
server(15-54)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
benchmark/performance.js(10 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
📚 Learning: 2025-11-08T13:46:04.917Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-09-21T15:43:32.265Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9858
File: src/GraphQL/ParseGraphQLServer.js:176-178
Timestamp: 2025-09-21T15:43:32.265Z
Learning: The GraphQL playground feature in ParseGraphQLServer.js (applyPlayground method) is intended for development environments only, which is why it includes the master key in client-side headers.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
benchmark/performance.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: Node 22
- GitHub Check: Node 18
- GitHub Check: Redis Cache
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Node 20
- GitHub Check: Docker Build
- GitHub Check: Benchmarks
🔇 Additional comments (3)
benchmark/performance.js (3)
30-32: LGTM: Clean logging abstraction for CI integration.The logging helpers provide a clean abstraction for GitHub Actions structured logging, improving visibility and consistency across the benchmark suite.
102-174: LGTM: Well-structured refactor with improved ergonomics.The options object pattern enhances flexibility and readability. The progress logging and conditional warmup are valuable additions for monitoring long-running benchmarks.
424-433: Verify whether all benchmarks should be enabled.All existing benchmarks are commented out, leaving only the new include benchmark active. Confirm whether this is intentional for testing or if the full suite should be re-enabled before merging.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
benchmark/performance.js (1)
370-419: Critical: Object creation still inside measured operation - past fix not applied.The past review comment (marked as "Addressed in commits 713a4d0 to 35f1809") flagged this exact issue, but the problem persists in the current code. Lines 384-412 create 30 objects (10×Level2 + 10×Level1 + 10×Root) inside the
operationfunction, so each iteration accumulates 30,000 objects with 1000 iterations, causing:
- Progressive query slowdown as the dataset grows
- Unreliable measurements (different dataset size per iteration)
- Memory and storage issues
- Skewed benchmark results that don't reflect the intended use case
The object setup must be moved outside
measureOperation, following the pattern inbenchmarkObjectReadandbenchmarkObjectUpdate. Only the query with include should be measured.Apply this refactor to move setup outside the measured operation:
async function benchmarkQueryWithInclude() { // Setup: Create nested object hierarchy const Level2Class = Parse.Object.extend('Level2'); const Level1Class = Parse.Object.extend('Level1'); const RootClass = Parse.Object.extend('Root'); + // Create 10 Level2 objects + const level2Objects = []; + for (let i = 0; i < 10; i++) { + const obj = new Level2Class(); + obj.set('name', `level2-${i}`); + obj.set('value', i); + level2Objects.push(obj); + } + await Parse.Object.saveAll(level2Objects); + + // Create 10 Level1 objects, each pointing to a Level2 object + const level1Objects = []; + for (let i = 0; i < 10; i++) { + const obj = new Level1Class(); + obj.set('name', `level1-${i}`); + obj.set('level2', level2Objects[i % level2Objects.length]); + level1Objects.push(obj); + } + await Parse.Object.saveAll(level1Objects); + + // Create 10 Root objects, each pointing to a Level1 object + const rootObjects = []; + for (let i = 0; i < 10; i++) { + const obj = new RootClass(); + obj.set('name', `root-${i}`); + obj.set('level1', level1Objects[i % level1Objects.length]); + rootObjects.push(obj); + } + await Parse.Object.saveAll(rootObjects); + return measureOperation({ name: 'Query with Include (2 levels)', - skipWarmup: true, dbLatency: 100, operation: async () => { - // Create 10 Level2 objects - const level2Objects = []; - for (let i = 0; i < 10; i++) { - const obj = new Level2Class(); - obj.set('name', `level2-${i}`); - obj.set('value', i); - level2Objects.push(obj); - } - await Parse.Object.saveAll(level2Objects); - - // Create 10 Level1 objects, each pointing to a Level2 object - const level1Objects = []; - for (let i = 0; i < 10; i++) { - const obj = new Level1Class(); - obj.set('name', `level1-${i}`); - obj.set('level2', level2Objects[i % level2Objects.length]); - level1Objects.push(obj); - } - await Parse.Object.saveAll(level1Objects); - - // Create 10 Root objects, each pointing to a Level1 object - const rootObjects = []; - for (let i = 0; i < 10; i++) { - const obj = new RootClass(); - obj.set('name', `root-${i}`); - obj.set('level1', level1Objects[i % level1Objects.length]); - rootObjects.push(obj); - } - await Parse.Object.saveAll(rootObjects); - const query = new Parse.Query('Root'); query.include('level1.level2'); await query.find(); }, }); }
🧹 Nitpick comments (3)
benchmark/MongoLatencyWrapper.js (2)
20-23: Consider scope and lifecycle of originalMethods Map.The module-scoped
originalMethodsMap could cause issues ifwrapMongoDBWithLatencyis called multiple times without proper unwrapping between calls. While the current usage pattern inperformance.jswraps and unwraps per benchmark, the wrapper doesn't guard against:
- Calling
wrapMongoDBWithLatencytwice without unwrapping (would lose the true originals)- Race conditions if multiple tests run concurrently
Consider adding a guard to prevent double-wrapping:
function wrapMongoDBWithLatency(latencyMs) { + if (originalMethods.size > 0) { + throw new Error('MongoDB is already wrapped. Call unwrap() before wrapping again.'); + } + if (typeof latencyMs !== 'number' || latencyMs < 0) { throw new Error('latencyMs must be a non-negative number'); }
65-71: Remove or clarify the synchronous fallback path (lines 65-71) as unreachable dead code.MongoDB Collection methods only return Promises or Cursors. All methods in
methodsToWrap(lines 94-116)—includingfind(),insertOne(),updateOne(), etc.—return either a FindCursor (handled at lines 43-54) or a Promise (handled at lines 57-63). The synchronous fallback (lines 65-71) will never execute. Either remove it or add a comment explaining it's a defensive safeguard that should never be reached.benchmark/performance.js (1)
25-25: Consider making LOG_ITERATIONS configurable via environment variable.While the current hard-coded
falsevalue is reasonable for typical use, allowing control via environment variable would provide flexibility for debugging without code changes:-const LOG_ITERATIONS = false; +const LOG_ITERATIONS = process.env.LOG_ITERATIONS === 'true';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
benchmark/MongoLatencyWrapper.js(1 hunks)benchmark/performance.js(9 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-08-26T14:06:31.853Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.853Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
benchmark/performance.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
benchmark/performance.js
🧬 Code graph analysis (2)
benchmark/performance.js (1)
benchmark/MongoLatencyWrapper.js (2)
require(20-20)result(40-40)
benchmark/MongoLatencyWrapper.js (2)
benchmark/performance.js (3)
require(15-15)require(16-16)require(17-17)src/Adapters/Storage/Mongo/MongoCollection.js (1)
Collection(2-2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Node 22
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: Node 20
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: Node 18
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Redis Cache
- GitHub Check: Docker Build
- GitHub Check: Benchmarks
🔇 Additional comments (7)
benchmark/MongoLatencyWrapper.js (2)
79-133: LGTM - Well-structured latency wrapper with proper cleanup.The implementation correctly:
- Validates input parameters
- Optimizes the zero-latency case
- Covers a comprehensive set of MongoDB Collection methods
- Provides proper cleanup via the unwrap function
The unwrap pattern ensures methods are restored and the Map is cleared, preventing state leakage between benchmark runs.
135-137: LGTM - Clean module interface.Exports only the public API, keeping internal implementation details (
wrapMethod,originalMethods) private.benchmark/performance.js (5)
13-13: LGTM - Appropriate use of GitHub Actions logging.The
@actions/corelibrary provides structured logging for CI environments and integrates well with the benchmark output format. The logging helpers provide a clean abstraction layer.Also applies to: 17-17, 32-33
99-101: LGTM - Useful utility for benchmark isolation.Resetting the Parse SDK URL between benchmarks ensures each benchmark starts from a clean state, preventing state leakage.
112-191: LGTM - Well-designed measurement infrastructure.The refactored
measureOperationprovides:
- Clean options-based API
- Per-benchmark latency control with proper cleanup in finally block
- Detailed progress logging for CI visibility
- Sound statistical analysis with IQR outlier filtering
The integration with
wrapMongoDBWithLatencycorrectly wraps before measurement and unwraps in the finally block, ensuring cleanup even on errors.
196-368: LGTM - Consistent migration to new API.All existing benchmarks correctly migrated to the new
measureOperationoptions-based API while preserving their original logic.
441-458: LGTM - Improved benchmark orchestration.The data-driven approach with the benchmarks array provides:
- Clear declaration of all benchmarks upfront
- Consistent isolation (reset + cleanup) before each benchmark
- Easier maintenance when adding/removing benchmarks
Signed-off-by: Manuel <5673677+mtrezza@users.noreply.github.com>
|
@Moumouls I was able to measure the performance improvement of this PR by introducing artificial latency to DB requests in the benchmark, see summary. And it behaves as you said, the higher the latency, the higher the improvement. 
The performance improvement applies to parallel includes (see benchmark code |
|
@mtrezza nested pointers are improved but in a specific way, the parallelization happen at each "nested level", so in case of p1.p2.p3 p3, p4, p5 are actually loaded in parallel because after p2 we know each objectId As mentioned in the description: parallelization potential happen at each level, because parse-server need to discover objectId before performing include calls. And currently i don't see an approach where you can get p1.p2.p3, without knowing p2 and p3 pointer ref. |
4 participants
Pull Request
Issue
Related to : #6501
Closes: #7981
Approach
I try to avoid tricks and weird pattern as much as possible.
The approach is "Plan and execute", so we build a tree, and then we execute using parallel promises, and we hydrate the tree using some references
Each branch is recursively executed independently as fast as possible, and new parallelism is introduced at each child node.
Results
Benchmark procedure: Create a Parse.Object with 10 points on it, then each child have also 10 points, resulting in 100 includes
Before optimization:
After optimization
Tasks
Summary by CodeRabbit
Release Notes
Performance
Tests